Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[iOS] Implement iOS PAL for S.S.C.X509Certificates #52191

Merged
merged 28 commits into from
May 19, 2021
Merged

[iOS] Implement iOS PAL for S.S.C.X509Certificates #52191

merged 28 commits into from
May 19, 2021

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented May 3, 2021 •
edited
Loading

The managed part is split from the macOS PAL implementation because it's substantially different. The chain building part is kept shared but certificate and store management is implemented from scratch with some code reuse. iOS keychain supports only one store that is exposed as the My store.

Missing features:

  • Root certificate store (not exposed by API)
  • PKCS [master] Update dependencies from dotnet/coreclr #7 import and export (not exposed by native API, can be added in managed code if deemed necessary)
  • Key store flags are ignored on certificate import (marked as TODO, matches current Android behavior)

It passes all the inner and outer loop tests except the ones that are explicitly disabled in the PR and the ones failing because of test runner issue (#52104; fix submitted in #52372).

Fixes #36897.
Fixes #49289.
Fixes #51388.
Contributes to #47910.
Contributes to #47533

@ghost
Copy link

ghost commented May 3, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Builds on top of #52043.

The managed part is split from the macOS PAL implementation because it's substantially different. The chain building part is kept shared but certificate and store management is implemented from scratch with some code reuse. iOS keychain supports only one store that is exposed as the names My store.

Missing features:

  • Root certificate store (not exposed by API)
  • PKCS [master] Update dependencies from dotnet/coreclr #7 import and export (not exposed by native API, can be added in managed code if deemed necessary)
  • Key store flags are ignored on certificate import (marked as TODO, matches current Android behavior)

Marked as draft for the moment to solicit early review.

Fixes #49289.
Contributes to #47910 and #47533

Author: filipnavara
Assignees: -
Labels:

area-System.Security
Milestone: -

@filipnavara filipnavara marked this pull request as draft May 3, 2021 15:15
@filipnavara filipnavara force-pushed the ios-x509-scratch branch 2 times, most recently from 3a66b0a to 146ce5d Compare May 4, 2021 16:25
@filipnavara
Copy link
Member Author

Rebased after #52043 was merged.

@filipnavara filipnavara marked this pull request as ready for review May 4, 2021 17:39
@steveisok steveisok requested a review from bartonjs May 5, 2021 01:34
@filipnavara filipnavara mentioned this pull request May 6, 2021
Closed
filipnavara
filipnavara commented May 7, 2021
View reviewed changes
src/libraries/tests.proj Outdated Show resolved Hide resolved
@steveisok
Copy link
Member

@bartonjs can you give this another pass?

@bartonjs
Copy link
Member

bartonjs commented May 10, 2021 •
edited
Loading

@bartonjs can you give this another pass?

It's on my todo list, currently around third, which probably means tomorrow afternoon.

bartonjs
bartonjs reviewed May 11, 2021
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only "real" concern before merging is the PEM header and content mismatch scenario (oh, and clearing the rented PEM buffer). Removing the highly repetitive triplet of platforms for the skip attributes is just bonus.

@filipnavara filipnavara requested a review from bartonjs May 12, 2021 16:33
@filipnavara
Copy link
Member Author

filipnavara commented May 12, 2021 •
edited
Loading

The one test failure is unrelated. (fixed by re-run) iOS Simulator tests passed on the runtime-staging pipeline.

@filipnavara filipnavara mentioned this pull request May 13, 2021
Closed
@filipnavara
Copy link
Member Author

@bartonjs Can you take another look please? I believe I have addressed all the issues you found in your last review.

@filipnavara filipnavara mentioned this pull request May 19, 2021
Closed
@bartonjs bartonjs merged commit d3e9a11 into dotnet:main May 19, 2021
@filipnavara filipnavara deleted the ios-x509-scratch branch May 19, 2021 16:35
@steveisok
Copy link
Member

@filipnavara thanks for the contribution!

@filipnavara
Copy link
Member Author

Happy to help and to cross it off the list. 😅

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akoeplinger akoeplinger akoeplinger left review comments

@bartonjs bartonjs bartonjs approved these changes

Assignees
No one assigned
Labels
area-System.Security
Projects
None yet
Milestone
6.0.0
5 participants
@filipnavara @steveisok @bartonjs @akoeplinger @karelz